-
Notifications
You must be signed in to change notification settings - Fork 1.5k
runsc: Add EROFS mount support in bundle config.json #12337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This change enables EROFS filesystems to be specified as regular mounts in the OCI bundle config.json, not just as rootfs or debug mounts. EROFS mounts are now tracked in goferMountConfs alongside lisafs mounts. The implementation adds: - IsErofsMount() helper in specutils to identify EROFS mounts - Updated mount index tracking across container.go, gofer.go, and vfs.go - Support for opening EROFS image files and passing FDs to the sandbox - EROFS case in getMountNameAndOptions() for proper mount setup Key implementation details: - EROFS mounts are included in goferMountConfs but skip gofer-specific processing (e.g., lisafs serving, filestore creation) - Mount type determination happens before mount hint logic - Proper index tracking ensures EROFS mounts increment indices but skip lisafs-only operations Fixes google#12307
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hello, gVisor devs! Can you review this PR please? |
|
Thanks for the PR @stepancheg, will have a look this week. |
ayushr2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests to runsc/container:container_test. We already have EROFS tests for debug mounts and rootfs:
gvisor/runsc/container/container_test.go
Line 3277 in 2773a02
| // createImageEROFS creates the EROFS image from the source directory using the requested options. |
| func IsErofsMount(m specs.Mount) bool { | ||
| return m.Type == erofs.Name | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think specutils should take a dependency on fsimpl/erofs for this == "erofs" check. specutils has very limited dependencies on the sentry. erofs pulls in a lot of stuff.
Similar to the duplication of the string "bind" above on line 503 and in
Line 70 in 2773a02
| Bind = "bind" |
m.Type == "erofs" and live with the duplication. It's not a big deal. In the future maybe we can move all these filesystem constants to a smaller high level package and refactor.
| if info.goferMountConf.ShouldUseLisafs() { | ||
| info.goferFD = c.goferFDs.removeAsFD() | ||
| } else if info.goferMountConf.ShouldUseErofs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use specutils.HasMountConf instead
| // EROFS mounts are in goferMountConfs but gofer doesn't set them up | ||
| if specutils.IsErofsMount(m) { | ||
| mountIdx++ | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use specutils.HasMountConfig instead
| // EROFS mounts are in goferMountConfs but gofer doesn't resolve them | ||
| if specutils.IsErofsMount(m) { | ||
| cleanMounts = append(cleanMounts, m) | ||
| mountIdx++ | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use specutils.HasMountConfig instead
| // EROFS mounts are in goferMountConfs but gofer doesn't serve them | ||
| if specutils.IsErofsMount(m) { | ||
| mountIdx++ | ||
| continue | ||
| } | ||
| if !specutils.IsGoferMount(m) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we really need is specutils.HasMountConfig(m).
// HasMountConfig returns true if the given mount has an associated GoferMountConf.
func HasMountConfig(m specs.Mount) bool {
return IsGoferMount(m) || IsErofsMount(m)
}
The logic below handles the rest correctly by continuing if !mountConf.ShouldUseLisafs()
| // EROFS mounts are in goferMountConfs but don't have self-backed filestores | ||
| if specutils.IsErofsMount(c.Spec.Mounts[i]) { | ||
| goferMntIdx++ | ||
| continue | ||
| } | ||
| if !specutils.IsGoferMount(c.Spec.Mounts[i]) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use specutils.HasMountConfig instead
| // Handle bind mounts. | ||
| for i := range c.Spec.Mounts { | ||
| if !specutils.IsGoferMount(c.Spec.Mounts[i]) { | ||
| if !specutils.IsGoferMount(c.Spec.Mounts[i]) && !specutils.IsErofsMount(c.Spec.Mounts[i]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use !specutils.HasMountConfig() instead
| } | ||
| c.GoferMountConfs = append(c.GoferMountConfs, goferConf) | ||
|
|
||
| // Handle bind mounts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update comment to say "Handle sub mounts."
EROFS mounts are not bind mounts.
| // EROFS mounts are in goferMountConfs but don't need filestore processing | ||
| if specutils.IsErofsMount(m) { | ||
| mountIdx++ | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this. In c.createGoferFilestore(), if !goferConf.IsFilestorePresent(), then we return immediately.
Moreover, in the future, we may want to support EROFS+overlay, for which we will need to create filestore for the upper layer.
| // Align spec mount with gofer mount conf by skipping non-gofer/non-erofs mounts | ||
| // Skip alignment for root mount (i == 0) because it is not present in spec.Mounts | ||
| if i > 0 { | ||
| for mountIdx < len(c.Spec.Mounts) && | ||
| !specutils.IsGoferMount(c.Spec.Mounts[mountIdx]) && | ||
| !specutils.IsErofsMount(c.Spec.Mounts[mountIdx]) { | ||
| mountIdx++ | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these changes here are really complex and hard to follow.
The c.GoferMountConfs list is a subset of c.Spec.Mounts. It is difficult to iterate configs and track mount index. Everywhere else (as you can see in this PR), we iterate the mounts and track the config index. That approach also requires handling rootfs separately (outside the main loop), but that should be OK:
- rootfs often requires special handling, as you can see below, we need to handle rootfs differently in the
if i==0branch. - It is consistent with the rest of the codebase.
So it should be something like:
switch {
case c.GoferMountConfs[0].ShouldUseLisafs():
refactored code to create socket pair and donate the gofer end and append the sandbox end
case c.GoferMountConfs[0].ShouldUseErofs():
f, err := os.Open(rootfsHint.Mount.Source)
if err != nil {
return nil, nil, nil, nil, fmt.Errorf("opening rootfs image %q: %v", rootfsHint.Mount.Source, err)
}
sandEnds = append(sandEnds, f)
}
cfgIdx := 1
for _, m := c.Spec.Mounts {
if !specutils.HasMountConfig(m) {
continue
}
switch {
case c.GoferMountConfs[cfgIdx].ShouldUseLisafs():
refactored code to create socket pair and donate the gofer end and append the sandbox end
case c.GoferMountConfs[cfgIdx].ShouldUseErofs():
f, err := os.Open(m.Source)
if err != nil {
return nil, nil, nil, nil, fmt.Errorf("opening EROFS image %q: %v", m.Source, err)
}
sandEnds = append(sandEnds, f)
}
cfgIdx++
}
This change enables EROFS filesystems to be specified as regular mounts in the OCI bundle config.json, not just as rootfs or debug mounts.
EROFS mounts are now tracked in goferMountConfs alongside lisafs mounts. The implementation adds:
Key implementation details:
Fixes #12307